Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve error handling and logic #58

Closed

Conversation

mcaskill
Copy link
Contributor

@mcaskill mcaskill commented Mar 8, 2023

Checklist

Draft 2 — 2023-03-15

Description

Follow-up to #52 and alternative to the initial draft (see below).

Primary focus is improving error handling by validating the response from cURL requests, validating the decoded/unserialized response bodies, and simplifying the logic between plugins.

How has this been tested?

I'm testing this on client projects that use ACF Pro, ACF Extended Pro, PublishPress Pro, Gravity Forms, Polylang Pro, and WPML.

Screenshots

Screen capture of the originally proposed error example "cURL error (22): The requested URL returned error: 404"

Screen capture of the latest proposed error example "JSON Error: Expected a data structure"

Types of changes

  • Plugins:
    • ACF Pro, Gravity Forms, WPML: Change URL formatting to use http_build_query() to improve readability and reduce risk of mistakes.
    • ACF Extended Pro, Gravity Forms, Polylang Pro: Change HTTP request method to use GET instead of POST to better match the verb of the request and for consistency between the other downloader. Both Gravity Forms' API and Easy Digital Downloads' API support GET requests.
    • Ninja Forms, PublishPress Pro, WPML: Throw InvalidArgumentException instead of UnexpectedValueException when dealing with an unsupported package.
    • EDD Plugins, Gravity Forms: Throw UnexpectedValueException if the decoded/unserialized API response is not an array.
    • EDD Plugins: Replaced getDownloadUrl() with a new protected method getDownloadUrlFromApi() that only handles the request for the download URL response object.
  • AbstractEddPlugin:
    • Replaced extractDownloadUrl() with getDownloadUrl() that retrieves the response to parse from the plugin's getDownloadUrlFromApi().
    • Wrapped requests with Http in a try/catch to intercept cURL exceptions and wrap them in a plugin-aware exception.
  • Http:
    • Added protected method request() to aggregate the execution of the request, handling of the response, and closing of the cURL handler.
    • Added cURL option CURLOPT_FAILONERROR to fail if the response is >= 400.
    • Throw RuntimeException if the request failed.
    • Compile URL in GET request earlier to organize cURL options similarly to POST request.
    • Use booleans intead of integers for cURL options.

Draft 1 — 2023-03-07

Description

Follow-up to #52.

Fixed the PSR-4 file name for Wpml class, for the sake of testing.

Improve checks on HTTP response. Check if status code is OK (200) and if the body is an array, throw exceptions if not with an excerpt of the response body for aid with debugging.

Expanding upon #47, improved Gravity Forms to check if download version matches the package's version with support for either available download: the "main version" (download_url) or the "latest version" (download_url_latest). For example:

  • version: 2.7.2 (PARADIGM.MAJOR.MINOR)
  • version_latest: 2.7.2.1 (PARADIGM.MAJOR.MINOR.PATCH)

As of 2023-03-08, this code sort of works but is not completely tested/vetted, nor is it the best design, and some of it should be split into their own pull requests.

If we drop support for Composer 1, a lot of this proposed code can be simplified to take advantage of Composer 2's utilities.

I'm open to comments, suggestions, and contributions.

How has this been tested?

I'm testing this on client projects that use ACF Pro, ACF Extended Pro, PublishPress Pro, Gravity Forms, Polylang Pro, and WPML.

Types of changes

  • Change URL formatting (for ACF, Gravity Forms, WPML) to use http_build_query() to improve readability and reduce risk of mistakes.
  • Change HTTP request method (for ACF Extended, Gravity Forms, Polylang) to use GET instead of POST to better match the verb of the request and for consistency between the other downloader. Both Gravity Forms' API and Easy Digital Downloads' API support GET requests.
  • Refactored Http class to store last response body and headers, allow parsing of body through callback, and finding of status code and message (based on Composer 2's Response, RemoteFileSystem, and HttpDownloader).
  • Use Composer's Semver to check Gravity Forms versions and throw an exception if we download the incorrect version.

@junaidbhura
Copy link
Owner

Looks good @mcaskill are you still working on this?

@mcaskill
Copy link
Contributor Author

mcaskill commented Mar 13, 2023

Thanks. Unfortunately, at this time, I won't be able to continue/finish work on this feature. If you or someone else wants to take over, I can add them to the branch.

Similarly, could you fix the WPML class (WPML.phpWpml.php) to resolve the open issue?

@mcaskill mcaskill force-pushed the feature/refactor-http-class branch from 5b3a2d9 to 05d1612 Compare March 15, 2023 15:12
Use `http_build_query()` to improve readability and reduce risk of errors.
Use GET instead of POST to better match the verb to the request and for consistency between downloaders.
Changed:
- Throw `InvalidArgumentException` instead of `UnexpectedValueException` when dealing with an unsupported package in Ninja Forms, PublishPress Pro, and WPML.
- Throw `UnexpectedValueException` if the decoded/unserialized API response is not an array.
- Improved messages to clarify errors from API.
- Fixed and improved PHPDoc tags to document exceptions.
Changed:
- Use booleans intead of integers for cURL options.
- Compile URL in GET request earlier to organize cURL options similarly to POST request.
Changed:
- Added protected method `request()` to aggregate the execution of the request, handling of the response, and closing of the cURL handler.
- Added cURL option `CURLOPT_FAILONERROR` to fail if the response is >= 400.
- Throw `RuntimeException ` if the request failed.
- Fixed and improved PHPDoc tags.
Changed:
- Replaced `getDownloadUrl()` in EDD plugins with a new protected method `getDownloadUrlFromApi()` that only handles the request for the download URL response object.
- Replaced `extractDownloadUrl()` with `getDownloadUrl()` in `AbstractEddPlugin` that retrieves the response to parse from the plugin's `getDownloadUrlFromApi()`.
- Wrapped requests with `Http` in a try/catch to intercept cURL exceptions and wrap them in a plugin-aware exception.
@mcaskill mcaskill force-pushed the feature/refactor-http-class branch from 05d1612 to cee1cf5 Compare March 16, 2023 00:41
@mcaskill mcaskill changed the title Improve HTTP response and error handling Improve error handling and improve logic Mar 16, 2023
@mcaskill mcaskill marked this pull request as ready for review March 16, 2023 00:42
@mcaskill
Copy link
Contributor Author

mcaskill commented Mar 16, 2023

I've updated the pull request title and description to reflect the latest state of proposed changes.

The initially proposed changes for Gravity Forms will be resubmitted as its own pull request once we've figured out this request.

Changed:
- Removed extraneous property descriptions.
- Rephrased description of `$slug` property to be more specific.
- Added missing `$slug` method parameter.
Amends cee1cf5

Accidentally reverted back from GET to POST.
Decoupled API URL and API URL query parameters from its HTTP query building and URL concatenation, and HTTP request call.

This allows the base URL and query parameters to be more readable from their final concatenated URL or HTTP request call.
This ensures compatibility with the classes' `$version` property being potentially empty, presuming a request for the latest version.
To reduce repetition, and risk of mistakes, when formatting the plugin's Composer package name. The package name is used in the messaging of most exceptions thrown.

Storing vendor name as a namespace
Changed:
- Moved shared cURL options to `request()` method.
- Catch any exception from `Http` requests.
Separated JSON decoding error handling from HTTP request/response error handling.

By using `json_last_error()`, we can provide a more precise error for end users as opposed to just checking if its an array.

Output a truncated excerpt of the response body if JSON decoding fails to give a hint of what the issue might be.
@mcaskill
Copy link
Contributor Author

mcaskill commented Jun 9, 2023

I found some time to do some further testing and improvements.

I think it should be functional enough.

If people can test, that would be appreciated.

@mcaskill mcaskill force-pushed the feature/refactor-http-class branch from c736634 to 03771dd Compare June 9, 2023 20:45
Separated unserialization error handling from HTTP request/response error handling.

Wrapped unserialization in try/catch to intercept any throwables from unexpected objects in their unserialization handlers. Gravity Forms is supposed to return an associative array but could be compromised.

Output a truncated excerpt of the response body if unserialization fails to give a hint of what the issue might be. Ideally, add an error handler to catch the PHP notice that is raised by `unserialize()`.
@mcaskill mcaskill force-pushed the feature/refactor-http-class branch from 03771dd to ab73a24 Compare June 9, 2023 20:46
@mcaskill mcaskill changed the title Improve error handling and improve logic Improve error handling and logic Jun 9, 2023
@mcaskill mcaskill closed this Jul 12, 2024
@mcaskill mcaskill deleted the feature/refactor-http-class branch July 12, 2024 22:31
mcaskill added a commit to wp-jazz/composer-wp-pro-plugins that referenced this pull request Jul 13, 2024
From 'mcaskill/feature/refactor-http-class':

Improve error handling and logic
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants